Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
package: angular
| // -------------------------------------------------- | ||
|
|
||
| .action-sheet-button-label { | ||
| gap: 12px; |
There was a problem hiding this comment.
I decided to use the same value that ionic uses.
| /** | ||
| * Text that is placed underneath the option text to provide additional details about the option. | ||
| */ | ||
| @Prop() description?: string; |
There was a problem hiding this comment.
Description isn't being used here to display on the screen because ion-select-option isn't used to display the options, it's done through the respective interface. So description is passed to the interface.
There was a problem hiding this comment.
In the ionic theme, when you open and close the modal from the icon it scrolls to the bottom of the view and also the focus indicator is cut off:
bug-with-select-multiple.mov
This does not happen for:
- The single value modal example
- The ios theme
- The md theme
- The overflowing or multiple basic select test on
next- However, the focus indicator issue is there on the multiple example
Note: I didn't test if this exact test has the issue on next since this test is new.
There was a problem hiding this comment.
I was not able to replicate either issues on Chrome and Firefox. What are the steps you took?
There was a problem hiding this comment.
Using Chrome or Firefox (does not happen in Safari):
- Navigate to https://ionic-framework-git-fw-7137-ionic1.vercel.app/src/components/select/test/rich-content-option?ionic:theme=ionic
- Click in the middle of the
Modalitem underMultiple Value - See that the modal opens with the scroll at the top
- Click on the select icon of the
Modalitem underMultiple Value - See that the modal is scrolled to the bottom and the last item is focused
- Refer to my video for visual steps
There was a problem hiding this comment.
Can we add screenshot tests for this? We need to check the visual rendering as well.
There was a problem hiding this comment.
The reason that I didn't create screenshots for this is because the content for the options are based on the project so they're not fixed. Would it be beneficial to create screenshots for something like that?
There was a problem hiding this comment.
We should define some good default styles for the rich content. As with slotted elements in input, it should look good out of the box without requiring additional styling from developers. I think we need to do this and then capture screenshots of it: Input: slot test
There was a problem hiding this comment.
The checkbox for the multiple value select modal looks different than the UX requirements - do we have a way for them to make this change?
| Our implementation | Figma requirements |
|---|---|
 |
 |
There was a problem hiding this comment.
That isn't part of the scope in my opinion. The ticket is meant to add HTML content to the options.
There was a problem hiding this comment.
I don't think we need to provide the styling out of the box, but we do need to make sure it's possible to achieve on this PR.
There was a problem hiding this comment.
The avatar size in md theme is way too large. Can we decrease it here to match the other themes?
There was a problem hiding this comment.
I would prefer to not modify the width since we'll eventually add sizes to native avatar and we can adjust it at that point. Let me know if you would still prefer to make the change now.
There was a problem hiding this comment.
We need to have good defaults for the UI when rich content is passed. See my comment here: #31072 (comment)
If we don't plan to adjust the styles now, we should create a follow-up ticket.
| test('should not have visual regressions when not expanded', async ({ page, skip }) => { | ||
| await page.goto(`/src/components/item-sliding/test/shapes`, config); | ||
|
|
||
| // TODO(FW-7288): Remove skip once fix has been implemented |
There was a problem hiding this comment.
I'm currently fixing the test so I don't expect this skip to be here long. I only added it so it's not a blocker.
brandyscarney
left a comment
brandyscarney
left a comment
There was a problem hiding this comment.
All of my feedback can be done in follow-up tickets so I am approving this. Great work! 💪
4 participants
Issue number: resolves #29890
What is the current behavior?
Select only allows plain text to be passed within it's options.
What is the new behavior?
ion-select-optioninnerHTMLTemplatesEnabledistruestartandendnamed slots for content that appears in the overlay interface but not in the selected textdescriptionprop for text rendered below the option label in the overlayion-selectstart/endslot contentaria-labelderives plain text only from the default slot, ensuring screen readers don't read slotted or element content--select-text-media-width,--select-text-media-height, etc.)Overlay interfaces (
alert,action-sheet,select-popover,select-modal)start,end,description, HTML label) consistently via the sharedrenderOptionLabelutilityActionSheetButton,AlertInput,SelectPopoverOption,SelectModalOption) are unchanged — rich content fields are on internal extended interfaces (SelectActionSheetButton,SelectAlertInput,SelectOverlayOption)sanitizeDOMStringbefore DOM injection to prevent XSSUtilities
select-option-render.tsx: shared render utility for option labels across all overlay componentsgetOptionContent: extracts and clones slot content fromion-select-optionfor overlays and selected textgetDefaultSlotPlainText— extracts plain text from the default slot, used for labels and ariaTests
Does this introduce a breaking change?
No, developers will be able to continue using plain text for select options as usual.
Other information
Preview